-
Notifications
You must be signed in to change notification settings - Fork 726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable some Clippy lints #4120
base: main
Are you sure you want to change the base?
Enable some Clippy lints #4120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for having a look at this.
I've added some comments and questions for some lints. I'm also not sure that we'll want to enable all of these. For example, the missing_const_for_fn
lint is explicitly in development and has a bunch of caveats. I think it might be better to open a PR for each lint. That would make it easier to say "yes we want this" or "no we don't want this" on a case-by-case basis.
Finally, for this to land, we'll need to fix the lints for all CI builds.
if t.len() == $length { | ||
if t.len() != $length { | ||
Err(wrong_tuple_length(t, $length)) | ||
} else { | ||
#[cfg(any(Py_LIMITED_API, PyPy, GraalPy))] | ||
return Ok(($(t.get_borrowed_item($n)?.extract::<$T>()?,)+)); | ||
|
||
#[cfg(not(any(Py_LIMITED_API, PyPy, GraalPy)))] | ||
unsafe {return Ok(($(t.get_borrowed_item_unchecked($n).extract::<$T>()?,)+));} | ||
} else { | ||
Err(wrong_tuple_length(t, $length)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lint looks like a false positive to me.
Something we could consider changing here would be to remove the return
statements and use implicit return. But with the two cfg
options, I'm not certain that'd be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had tried that initially, but explicit return seems to be required:
697 | #[cfg(any(Py_LIMITED_API, PyPy, GraalPy))]
| ------------------------------------------ only `;` terminated statements or tail expressions are allowed after this attribute
698 | Ok(($(t.get_borrowed_item($n)?.extract::<$T>()?,)+))
| ^ expected `;` here
src/panic.rs
Outdated
} else if let Some(s) = payload.downcast_ref::<&str>() { | ||
Self::new_err((s.to_string(),)) | ||
Self::new_err(((*s).to_string(),)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better solution here would be to use payload.downcast::<&str>
. I think downcast_ref
introduces the extra reference clippy is complaining about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this?
if let Some(string) = payload.downcast_ref::<String>() {
Self::new_err((string.clone(),))
} else if let Ok(s) = payload.downcast::<&str>() { // <-- downcast / Ok
Self::new_err((s.to_string(),)) // <-- no ref
} else {
Self::new_err(("panic from Rust code",))
}
Edit: I pushed this change in.
@@ -152,7 +152,7 @@ impl From<Bound<'_, PyBytes>> for PyBackedBytes { | |||
let b = py_bytes.as_bytes(); | |||
let data = NonNull::from(b); | |||
Self { | |||
storage: PyBackedBytesStorage::Python(py_bytes.to_owned().unbind()), | |||
storage: PyBackedBytesStorage::Python(py_bytes.clone().unbind()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this one - maybe to_owned
is more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an artefact from refactoring the initial implementation and clone
seems appropriate, i.e. we have a PyBytes
and just want another reference to it, not change from a borrowed to an owned type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint description says that there is no difference, and to_owned
is misleading because there is no change in ownership.
I dropped the const and semicolon lints, since those changed a lot of lines and were both failing in CI (hard to check locally because of all the various config flags). I can still break up the remaining commits if you like, but I think it should be a lot more manageable now. |
I didn't realize there was such deviation between a local run and a CI run 😵💫 |
You can reproduce most of that locally using the Nox sessions which e.g. enumerate feature sets. |
Especially since what could be Edit: I see it's allow-by-default. That makes more sense, and once it's out of the nursery it will probably go to a lint group that we definitely don't want to have in a sweeping |
I'm still not convinced we necessarily want all of these, so I'd prefer to have them as separate PRs. |
Hello! This PR enables some Clippy lints and fixes all the warnings. There is one lint enabled per commit (except for the
pyo3-macros
) one, so I would suggest reviewing the commits one at a time. The changes all seem reasonable, and hopefully not too intrusive. It doesn't dopyo3-macros-backend
, but I can add that too if that's wanted.